Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style link id cache #3136

Closed
wants to merge 2 commits into from
Closed

Style link id cache #3136

wants to merge 2 commits into from

Conversation

rodrigogiraoserrao
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao commented Sep 26, 2023

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

The changes proposed here pass all tests.
No regression tests have been added yet.

Description

This follows Textualize/textual#1587 and #2949.
The issue we're dealing with boils down to the fact that the attribute Style._link_id is not taken into account when hashing a Style which then introduces caching collisions with styles that have the same _link/_meta attributes but that have a different _link_id.

Can't we include _link_id in the hash of a style?

We can't, because we don't want equality comparison to depend on _link_id, and if two objects are compared as equal, then their hash should be the same.

When working on Textualize/textual#1587, the first fix I found was that I could disable the cache inside rich.text.Text.render and the functools.lru_cache around rich.style.Style._add and that would fix the issue.
So, this does show it is a caching issue.
In particular, it has to do with the clashes that can exist when styles have the same meta values but originally have different _link_id attributes:

from rich.style import Style
meta = {"click": "something"}
s1 = Style.from_meta(meta)
s2 = Style.from_meta(meta)
print(s1.link_id, s2.link_id)  # Different link_id.
# (
#     '6311506187629892456089155',
#     '4273756187629892456089155'
# )

base = Style(color="red")
print((base + s1).link_id)  # This is the link_id of s1.
# '6311506187629892456089155'

# Now, we add base to s2:
#             vv
print((base + s2).link_id)  # This is the link_id of s1!
# '6311506187629892456089155'

Now, we can take a look at how addition is implemented and this reveals a promising workaround:

class Style:
    # ...
    def __add__(self, style: Optional["Style"]) -> "Style":
        combined_style = self._add(style)
        return combined_style.copy() if combined_style.link else combined_style

The call to .copy() refreshes the link_id if the resulting combined style has a link.
So, one might think it is a good idea to change the return statement to

return combined_style.copy() if combined_style.link or combined_style.meta else combined_style

However, this breaks link highlighting on the Textual side entirely – as in, no link gets any highlighting – because Textual relies on the link IDs to know what to highlight.
When the markup is first parsed, Textual will keep tabs on the link IDs of the styles it created, so if the rendering of the widget will come back with a style with a new link ID, Textual won't know what to highlight.

A potential fix is to bypass the cache entirely when adding styles that have link IDs but no links.

Working on Textualize/textual#1587 surfaced a caching issue that is best shown by running the code below:

```py
from rich.style import Style

meta = {"click": "something"}
s1 = Style.from_meta(meta)
s2 = Style.from_meta(meta)
print(s1.link_id, s2.link_id)  # Different link_id.

base = Style(color="red")
print((base + s1).link_id)  # This is the link_id of s1.
print((base + s2).link_id)  # This is the link_id of s1!
```

The change presented here will bypass cache when adding styles that have a link id but don't have a link attribute (if they did, so would the combined style and the call to .copy would refresh the link id either way).
Simply refreshing the link id will break Textual link highlighting.
For a reason similar to that of 7fbcc6d, we need to use the style indices as cache keys here and not the styles themselves because styles that have the same link/meta will have different link IDs, but have the same hash, so there will be cache collisions that we need to avoid.
@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (aabfd16) 98.30% compared to head (7eb7c92) 98.28%.
Report is 15 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3136      +/-   ##
==========================================
- Coverage   98.30%   98.28%   -0.03%     
==========================================
  Files          74       74              
  Lines        8038     8049      +11     
==========================================
+ Hits         7902     7911       +9     
- Misses        136      138       +2     
Flag Coverage Δ
unittests 98.28% <96.15%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
rich/console.py 98.04% <100.00%> (+<0.01%) ⬆️
rich/markdown.py 99.45% <100.00%> (ø)
rich/style.py 99.53% <100.00%> (+<0.01%) ⬆️
rich/syntax.py 98.19% <100.00%> (-0.29%) ⬇️
rich/text.py 99.01% <87.50%> (-0.17%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rodrigogiraoserrao
Copy link
Contributor Author

@willmcgugan if I recall correctly, when I tackled this we talked a bit and concluded this wasn't trivial. This PR was a draft because it was a proposed solution.
I'll be happy to mark it as ready if you prefer that to review it.

@willmcgugan
Copy link
Collaborator

If you think it is a possible solution, please mark it as ready. We'll go over it at some point. Need to refresh my memory.

@rodrigogiraoserrao rodrigogiraoserrao marked this pull request as ready for review February 1, 2024 15:53
@willmcgugan
Copy link
Collaborator

I don't quite grok this enough to merge. Closing for now, will look in to it again when I tackle the original issue.

@willmcgugan willmcgugan closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants